fix: less time intensive slashing migration #580
Conversation
WalkthroughThe changes involve introducing a mechanism to handle deprecated missed block bit arrays in the slashing module, aiming to improve performance and maintainability. A global variable sets a pruning limit per block, and a new function facilitates the deletion of deprecated entries. The migration script updates key prefixes and ensures backward compatibility without causing chain downtime during upgrades. These modifications signify an effort to optimize data storage and access patterns within the blockchain framework. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -11,6 +11,8 @@ import ( | |||
| "github.com/cosmos/cosmos-sdk/x/slashing/types" | |||
| ) | |||
|
|
|||
| var deprecatedBitArrayPruneLimitPerBlock = 2000 | |||
There was a problem hiding this comment.
I could be convinced to lower this, but I would rather have the one time payment of deleting every block frontloaded at time of upgrade rather than slight degradation for a very long time.
| @@ -115,3 +116,43 @@ func (k Keeper) deleteAddrPubkeyRelation(ctx sdk.Context, addr cryptotypes.Addre | |||
| store := ctx.KVStore(k.storeKey) | |||
| store.Delete(types.AddrPubkeyRelationKey(addr)) | |||
| } | |||
|
|
|||
| func (k Keeper) DeleteDeprecatedValidatorMissedBlockBitArray(ctx sdk.Context, iterationLimit int) { | |||
| store := ctx.KVStore(k.storeKey) | |||
There was a problem hiding this comment.
I was going between using store vs using params a few times. I thought params was the best at first since we already call getParams in the BeginBlocker, but it felt kind of awkward.
x/slashing/keeper/keeper.go
Outdated
| // created anonymous function to scope defer statement | ||
| func() { | ||
| iter := storetypes.KVStorePrefixIterator(store, v4.DeprecatedValidatorMissedBlockBitArrayPrefixKey(address)) | ||
| defer iter.Close() | ||
|
|
||
| for ; iter.Valid(); iter.Next() { | ||
| store.Delete(iter.Key()) | ||
| iterationCounter++ | ||
| if iterationCounter >= iterationLimit { | ||
| break | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Using anon func due to nested defers
| // NOTE: sdk v0.50 uses the same key prefix for both deprecated and new missed block bitmaps. | ||
| // We needed to use a new key, because we are skipping deletion of all old keys at upgrade time | ||
| // due to how long this would bring the chain down. We use 0x10 here to prevent overlap with any future keys. | ||
| validatorMissedBlockBitMapKeyPrefix = []byte{0x10} |
There was a problem hiding this comment.
Set to 0x10 to prevent overlap if more keys are introduced in the next sdk release we upgrade to
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- x/slashing/abci.go (2 hunks)
- x/slashing/keeper/keeper.go (2 hunks)
- x/slashing/migrations/v4/keys.go (2 hunks)
- x/slashing/migrations/v4/migrate.go (4 hunks)
- x/slashing/migrations/v4/migrate_test.go (3 hunks)
- x/slashing/types/keys.go (1 hunks)
Additional comments: 9
x/slashing/abci.go (2)
- 14-14: The global variable
deprecatedBitArrayPruneLimitPerBlockis set to 2000, which is used to limit the number of records deleted per block during the migration process. This approach helps in spreading the deletion process over multiple blocks, enhancing performance and stability.- 30-30: The addition of
DeleteDeprecatedValidatorMissedBlockBitArraycall within theBeginBlockerfunction is a significant change. It ensures that entries for the deprecated MissedBlockBitArray are deleted up to the per-block limit set bydeprecatedBitArrayPruneLimitPerBlock. This incremental deletion approach is crucial for maintaining blockchain stability and performance during the migration.x/slashing/migrations/v4/keys.go (1)
- 14-20: The update to key prefixes and the introduction of a new key prefix (
validatorMissedBlockBitMapKeyPrefix) for the missed block bitmaps are crucial for the migration logic. The choice of0x10for the new key prefix, as mentioned, is to prevent overlap with future keys, which is a thoughtful consideration for future compatibility.x/slashing/migrations/v4/migrate_test.go (2)
- 37-37: Replacing references to
v4.ValidatorMissedBlockBitArrayKeywithv4.DeprecatedValidatorMissedBlockBitArrayKeyin the test setup is aligned with the migration changes. This ensures that the test accurately reflects the migration logic by focusing on deprecated data structures.- 47-49: The test now checks for empty chunks instead of specific missed blocks, reflecting the migration's aim to reset validators' missed blocks. This simplification is in line with the migration's objectives to improve upgrade performance by not carrying over detailed missed block information.
x/slashing/types/keys.go (1)
- 55-58: Updating the
ValidatorMissedBlockBitmapKeyPrefixto0x10and introducingIsPruningKeyandTrueByteValueare significant changes that support the migration logic. The new key prefix aligns with the migration strategy, and the additional keys (IsPruningKeyandTrueByteValue) are likely to be used for tracking the migration's progress and state.x/slashing/migrations/v4/migrate.go (2)
- 25-30: Resetting all validators' missed blocks to improve upgrade performance is a strategic decision that aligns with the migration's objectives. By opting to not carry over detailed missed block information, the migration process is simplified and potentially accelerated.
- 44-47: Skipping the deletion of the deprecated validator missed block bit array and setting the
IsPruningKeyto indicate pruning in progress are crucial steps in the migration process. These actions ensure that the migration can be performed incrementally, spreading the load over multiple blocks, which is essential for maintaining blockchain performance.x/slashing/keeper/keeper.go (1)
- 120-159: The introduction of
DeleteDeprecatedValidatorMissedBlockBitArrayfunction in theKeeperstruct is a key part of the migration logic. This function iteratively deletes deprecated missed block bit arrays, adhering to an iteration limit per block. This approach is consistent with the incremental deletion strategy discussed earlier, ensuring that the blockchain's performance is not adversely affected during the migration process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 24
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 47-47: The entry for PR fix: less time intensive slashing migration #580 is concise and correctly placed under the "State Breaking" section, aligning with the PR's objectives and changes.
| * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block | ||
| * (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block | ||
| * (gov) [#514](https://github.com/osmosis-labs/cosmos-sdk/pull/514) Let gov hooks return an error | ||
| * (slashing) [#580](https://github.com/osmosis-labs/cosmos-sdk/pull/580) Less time intensive slashing migration | ||
|
|
||
| ## [State Compatible] | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [42-42]
The term "CacheKV speedups" might be more accurately described as "CacheKV performance improvements" to clearly convey the nature of the changes.
- * (store) [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV speedups
+ * (store) [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV performance improvements📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [51-51]
The term "Removal of regex usage on denom validation" could be more descriptive. Suggesting: "Improve denom validation by removing regex usage".
- * (coin) [#570](https://github.com/osmosis-labs/cosmos-sdk/pull/570) Removal of regex usage on denom validation
+ * (coin) [#570](https://github.com/osmosis-labs/cosmos-sdk/pull/570) Improve denom validation by removing regex usage📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [59-59]
The entry "Bumps IAVL v1 version for bug fixes" could be reworded for clarity and consistency with other entries. Suggestion: "Update IAVL v1 version to include bug fixes".
- * (store) [#575](https://github.com/osmosis-labs/cosmos-sdk/pull/575) Bumps IAVL v1 version for bug fixes
+ * (store) [#575](https://github.com/osmosis-labs/cosmos-sdk/pull/575) Update IAVL v1 version to include bug fixes📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [72-72]
The phrase "Speedup to UnmarshalBalanceCompat" could be improved for clarity. Suggestion: "Enhance performance of UnmarshalBalanceCompat".
- * (x/bank) [#547](https://github.com/osmosis-labs/cosmos-sdk/pull/547) Speedup to UnmarshalBalanceCompat
+ * (x/bank) [#547](https://github.com/osmosis-labs/cosmos-sdk/pull/547) Enhance performance of UnmarshalBalanceCompat📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [78-78]
The entry "Fix IAVL db grow issue" could be more descriptive. Suggestion: "Address IAVL database growth issue".
- * (store) [#537](https://github.com/osmosis-labs/cosmos-sdk/pull/537) Fix IAVL db grow issue
+ * (store) [#537](https://github.com/osmosis-labs/cosmos-sdk/pull/537) Address IAVL database growth issue📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [100-100]
The term "Speedup to UnmarshalBalanceCompat" could be rephrased for better clarity. Suggestion: "Enhance UnmarshalBalanceCompat performance".
- * (x/bank) [#546](https://github.com/osmosis-labs/cosmos-sdk/pull/546) Speedup to UnmarshalBalanceCompat
+ * (x/bank) [#546](https://github.com/osmosis-labs/cosmos-sdk/pull/546) Enhance UnmarshalBalanceCompat performance📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [109-109]
The entry "Add QueryEventForTxCmd cmd to subscribe and wait event for transaction by hash." could be rephrased for clarity. Suggestion: "Introduce QueryEventForTxCmd command for event subscription and waiting based on transaction hash."
- * (client/rpc) [#17274](https://github.com/cosmos/cosmos-sdk/pull/17274) Add `QueryEventForTxCmd` cmd to subscribe and wait event for transaction by hash.
+ * (client/rpc) [#17274](https://github.com/cosmos/cosmos-sdk/pull/17274) Introduce `QueryEventForTxCmd` command for event subscription and waiting based on transaction hash.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [114-114]
The phrase "Add MsgSubmitProposal SetMsgs method." could be more descriptive. Suggestion: "Introduce SetMsgs method in MsgSubmitProposal."
- * (x/gov) [#17387](https://github.com/cosmos/cosmos-sdk/pull/17387) Add `MsgSubmitProposal` `SetMsgs` method.
+ * (x/gov) [#17387](https://github.com/cosmos/cosmos-sdk/pull/17387) Introduce `SetMsgs` method in `MsgSubmitProposal`.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [115-115]
The entry "Emit VoterAddr in proposal_vote event." could be rephrased for clarity. Suggestion: "Include VoterAddr in the proposal_vote event."
- * (x/gov) [#17354](https://github.com/cosmos/cosmos-sdk/issues/17354) Emit `VoterAddr` in `proposal_vote` event.
+ * (x/gov) [#17354](https://github.com/cosmos/cosmos-sdk/issues/17354) Include `VoterAddr` in the `proposal_vote` event.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [117-117]
The phrase "Add MigrateHandler to allow reuse migrate genesis related function." could be improved for grammatical accuracy. Suggestion: "Introduce MigrateHandler for reusable migration of genesis-related functions."
- * (x/genutil) [#17296](https://github.com/cosmos/cosmos-sdk/pull/17296) Add `MigrateHandler` to allow reuse migrate genesis related function.
+ * (x/genutil) [#17296](https://github.com/cosmos/cosmos-sdk/pull/17296) Introduce `MigrateHandler` for reusable migration of genesis-related functions.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [123-123]
The phrase "Properly allow to combine depinject-enabled modules and non-depinject-enabled modules in app v2." could be rephrased for clarity. Suggestion: "Enable seamless integration of depinject-enabled and non-depinject-enabled modules in app v2."
- * (runtime) [#17284](https://github.com/cosmos/cosmos-sdk/pull/17284) Properly allow to combine depinject-enabled modules and non-depinject-enabled modules in app v2.
+ * (runtime) [#17284](https://github.com/cosmos/cosmos-sdk/pull/17284) Enable seamless integration of depinject-enabled and non-depinject-enabled modules in app v2.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [126-126]
The phrase "Do not try validate msgURL as web URL in draft-proposal command." could be improved for grammatical accuracy. Suggestion: "Avoid validating msgURL as a web URL in the draft-proposal command."
- * (x/gov,x/group) [#17220](https://github.com/cosmos/cosmos-sdk/pull/17220) Do not try validate `msgURL` as web URL in `draft-proposal` command.
+ * (x/gov,x/group) [#17220](https://github.com/cosmos/cosmos-sdk/pull/17220) Avoid validating `msgURL` as a web URL in the `draft-proposal` command.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [138-138]
The phrase "Improve simd prune UX by using the app default home directory and set pruning method as first variable argument (defaults to default)." could be rephrased for clarity. Suggestion: "Enhance simd prune user experience by utilizing the app's default home directory and setting the pruning method as the primary variable argument."
- * (cli) [#16856](https://github.com/cosmos/cosmos-sdk/pull/16856) Improve `simd prune` UX by using the app default home directory and set pruning method as first variable argument (defaults to default).
+ * (cli) [#16856](https://github.com/cosmos/cosmos-sdk/pull/16856) Enhance `simd prune` user experience by utilizing the app's default home directory and setting the pruning method as the primary variable argument.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [154-154]
The phrase "Add circuit breaker setter in baseapp." could be rephrased for clarity. Suggestion: "Introduce circuit breaker setter functionality in BaseApp."
- * (baseapp) [#16290](https://github.com/cosmos/cosmos-sdk/pull/16290) Add circuit breaker setter in baseapp.
+ * (baseapp) [#16290](https://github.com/cosmos/cosmos-sdk/pull/16290) Introduce circuit breaker setter functionality in BaseApp.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [156-156]
The phrase "Add WithExtensionOptions in tx Factory to allow SetExtensionOptions with given extension options." could be rephrased for clarity. Suggestion: "Introduce WithExtensionOptions in the transaction Factory to enable setting extension options."
- * (tx) [#15992](https://github.com/cosmos/cosmos-sdk/pull/15992) Add `WithExtensionOptions` in tx Factory to allow `SetExtensionOptions` with given extension options.
+ * (tx) [#15992](https://github.com/cosmos/cosmos-sdk/pull/15992) Introduce `WithExtensionOptions` in the transaction Factory to enable setting extension options.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [160-160]
The phrase "Make DefaultProposalHandler.ProcessProposalHandler return a ProcessProposal NoOp when using none or a NoOp mempool." could be rephrased for clarity. Suggestion: "Ensure DefaultProposalHandler.ProcessProposalHandler returns a NoOp for ProcessProposal when using a NoOp or empty mempool."
- * (baseapp) [#16407](https://github.com/cosmos/cosmos-sdk/pull/16407) Make `DefaultProposalHandler.ProcessProposalHandler` return a ProcessProposal NoOp when using none or a NoOp mempool.
+ * (baseapp) [#16407](https://github.com/cosmos/cosmos-sdk/pull/16407) Ensure `DefaultProposalHandler.ProcessProposalHandler` returns a NoOp for ProcessProposal when using a NoOp or empty mempool.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [163-163]
The phrase "Use cosmossdk.io/log package for logging instead of CometBFT logger." could be rephrased for clarity. Suggestion: "Adopt cosmossdk.io/log for logging, replacing the CometBFT logger."
- * (server) [#15984](https://github.com/cosmos/cosmos-sdk/pull/15984) Use `cosmossdk.io/log` package for logging instead of CometBFT logger.
+ * (server) [#15984](https://github.com/cosmos/cosmos-sdk/pull/15984) Adopt `cosmossdk.io/log` for logging, replacing the CometBFT logger.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [168-168]
The phrase "Update simulation to allow non-EOA accounts to stake." could be rephrased for clarity. Suggestion: "Modify simulation to enable staking for non-EOA accounts."
- * (x/staking) [#16068](https://github.com/osmosis-labs/cosmos-sdk/pull/16068) Update simulation to allow non-EOA accounts to stake.
+ * (x/staking) [#16068](https://github.com/osmosis-labs/cosmos-sdk/pull/16068) Modify simulation to enable staking for non-EOA accounts.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [170-170]
The phrase "Rename interface ExtensionOptionI back to TxExtensionOptionI to avoid breaking change." could be rephrased for clarity. Suggestion: "Revert interface name from ExtensionOptionI to TxExtensionOptionI to prevent breaking changes."
- * (types) [#16145](https://github.com/cosmos/cosmos-sdk/pull/16145) Rename interface `ExtensionOptionI` back to `TxExtensionOptionI` to avoid breaking change.
+ * (types) [#16145](https://github.com/cosmos/cosmos-sdk/pull/16145) Revert interface name from `ExtensionOptionI` to `TxExtensionOptionI` to prevent breaking changes.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [171-171]
The phrase "Add Close method to BaseApp for custom app to cleanup resource in graceful shutdown." could be rephrased for clarity. Suggestion: "Introduce Close method in BaseApp for resource cleanup during graceful shutdown of custom apps."
- * (baseapp) [#16193](https://github.com/cosmos/cosmos-sdk/pull/16193) Add `Close` method to `BaseApp` for custom app to cleanup resource in graceful shutdown.
+ * (baseapp) [#16193](https://github.com/cosmos/cosmos-sdk/pull/16193) Introduce `Close` method in `BaseApp` for resource cleanup during graceful shutdown of custom apps.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [176-176]
The phrase "Do not override some Comet config is purposely set differently in InterceptConfigsPreRunHandler." could be rephrased for clarity. Suggestion: "Avoid overriding specific Comet configurations that are intentionally set differently in InterceptConfigsPreRunHandler."
- * (server) [#16395](https://github.com/cosmos/cosmos-sdk/pull/16395) Do not override some Comet config is purposely set differently in `InterceptConfigsPreRunHandler`.
+ * (server) [#16395](https://github.com/cosmos/cosmos-sdk/pull/16395) Avoid overriding specific Comet configurations that are intentionally set differently in `InterceptConfigsPreRunHandler`.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [177-177]
The phrase "Fix StateSync Restore by excluding memory store." could be rephrased for clarity. Suggestion: "Correct StateSync restoration process by excluding the memory store."
- * (store) [#16449](https://github.com/cosmos/cosmos-sdk/pull/16449) Fix StateSync Restore by excluding memory store.
+ * (store) [#16449](https://github.com/cosmos/cosmos-sdk/pull/16449) Correct StateSync restoration process by excluding the memory store.📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [178-178]
The phrase "Allow any addresses in client.ValidatePromptAddress." could be rephrased for clarity. Suggestion: "Enable validation for all addresses in client.ValidatePromptAddress."
- * (cli) [#16312](https://github.com/cosmos/cosmos-sdk/pull/16312) Allow any addresses in `client.ValidatePromptAddress`.
+ * (cli) [#16312](https://github.com/cosmos/cosmos-sdk/pull/16312) Enable validation for all addresses in `client.ValidatePromptAddress`.| @@ -44,6 +44,7 @@ | |||
| * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block | |||
| * (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block | |||
There was a problem hiding this comment.
The phrase "Make slashing not write sign info every block" could be rephrased for clarity. Suggestion: "Optimize slashing by reducing sign info writes".
- * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block
+ * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Optimize slashing by reducing sign info writesCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block | |
| * (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block | |
| * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Optimize slashing by reducing sign info writes |
Description
Closes: #XXXX
Updates slashing migration logic to:
Testing
I did a broad test with the in-place-testnet and all seemed well. When we do a more thorough upgrade test with multiple validators, we should bring a validator down and ensure the counter incs as expected.
Just b/c it took me a little to figure this out let me write it here:
You get the validator address like:
osmosisd tendermint show-validatorAnd then get signing info like:
osmosisd q slashing signing-info '{"@type":"/cosmos.crypto.ed25519.PubKey","key":"kY2Q8mLJkMgK9aFfZlvfllUiiP0D+h7HC+3hH87Qosk="}'Summary by CodeRabbit